PHPLIB-1440: Sync range v2 spec tests to latest version #1373
Conversation
This syncs spec tests to mongodb/specifications@f5a1899
| }, | ||
| "result": { | ||
| "ok": 1 |
There was a problem hiding this comment.
I removed a similar assertion in fle2v2-Rangev2-Compact as that test failed due to a type mismatch:
[2024/09/04 10:33:57.215] Failed asserting that two strings are equal.
[2024/09/04 10:33:57.215] --- Expected
[2024/09/04 10:33:57.215] +++ Actual
[2024/09/04 10:33:57.215] @@ @@
[2024/09/04 10:33:57.215] -'{ "ok" : 1 }'
[2024/09/04 10:33:57.215] +'{ "ok" : 1.0 }'
[2024/09/04 10:33:57.215] /data/mci/f802858d094039b002289e9b38f42a7b/src/tests/TestCase.php:116
[2024/09/04 10:33:57.215] /data/mci/f802858d094039b002289e9b38f42a7b/src/tests/SpecTests/ResultExpectation.php:252
[2024/09/04 10:33:57.215] /data/mci/f802858d094039b002289e9b38f42a7b/src/tests/SpecTests/Operation.php:205
[2024/09/04 10:33:57.215] /data/mci/f802858d094039b002289e9b38f42a7b/src/tests/SpecTests/ClientSideEncryptionSpecTest.php:210
Doesn't the absence of a result field imply that the command should at least succeed, but no assertions on the result should be made? If so, then this result expectation isn't really necessary, as the command failing should trip up test runners regardless. Note that this is the legacy CSFLE test runner, so its behaviour may be a little more undefined than the unified test runner.
If this expectation is not necessary, I'd update the spec tests; if it is we'd have to update our matcher to ignore the type difference. What do you think @jmikola?
There was a problem hiding this comment.
Not sure where the ambiguity is coming from here (I know mongod and mongos used to differ in what they used for ok), but I assume PHPLIB's legacy test runner isn't handling flexible numeric comparisons as we mandate in the unified runner.
The runCommand spec doesn't talk about throwing in the event of an unsuccessful response (i.e. ok is not 1 or 1.0), but I'd hope we can trust the legacy test runner to throw in the event of an unsuccessful command. Would be good to run this by @kevinAlbs if you wanted to remove the result expectation entirely.
Otherwise, we can consider adding flexible numeric comparisons to PHPLIB's legacy runner.
There was a problem hiding this comment.
Doesn't the absence of a
resultfield imply that the command should at least succeed
mongodb/specifications#1605 noted:
Adding
result: { ok: 1 }to the operation is to ensure test runners check the operation succeeded. The test format does not require checking the operation succeeded ifresultis not present.
As a result, a bug in libmongocrypt (MONGOCRYPT-699) had gone unnoticed (at least in libmongoc's legacy test runner).
There was a problem hiding this comment.
Thanks for clarifying! I'll update our legacy test runner accordingly.
alcaeus
force-pushed
the
phplib-1440-range-ga
branch
from
8146822 to
88bf036
Compare
| Document::fromPHP($normalizedExpectedDocument)->toRelaxedExtendedJSON(), | ||
| Document::fromPHP($normalizedActualDocument)->toRelaxedExtendedJSON(), | ||
| ); | ||
| (new DocumentsMatchConstraint($expectedDocument, true, true))->evaluate($actualDocument); |
There was a problem hiding this comment.
Noted both parameters are to ignore extra keys. The purpose of this method is preserved.
https://github.com/mongodb/mongo-php-library/blob/70f8704ddc59fc25b085f8c98945f267506989dd/tests/SpecTests/DocumentsMatchConstraint.php#L69C47-L69C68
PHPLIB-1440
This syncs spec tests to mongodb/specifications@f5a1899